Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add GeoJSONOverlay #1082

Merged
merged 5 commits into from
Dec 8, 2023
Merged

add GeoJSONOverlay #1082

merged 5 commits into from
Dec 8, 2023

Conversation

miles-grant-ibigroup
Copy link
Collaborator

Description:

Adds a new overlay that renders any geojson on the map!

to configure:

 - type: geojson
      name: Custom Layer
      visible: true
      url: <geojson url here>

PR Checklist:

  • Does the code follow accessibility standards (WCAG 2.1 AA Compliant)?
  • Are all languages supported (Internationalization/Localization)?
  • Are appropriate Typescript types implemented?

amy-corson-ibigroup
amy-corson-ibigroup approved these changes Dec 5, 2023
View reviewed changes
Copy link
Contributor

@amy-corson-ibigroup amy-corson-ibigroup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to work well! I'm wondering though, aren't all these map layers geoJSON? Would it make sense to call this like "custom map layer"? Your call!

lib/components/map/connected-geojson-layer.js Outdated
<img
alt={properties.Name}
src={properties.icon}
style={{ height: 40 }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is coming across as very tall!!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree let's remove it

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a className should still be set on the <img> tag so that the configs can style the images if needed.

binh-dam-ibigroup
binh-dam-ibigroup requested changes Dec 6, 2023
View reviewed changes
Copy link
Collaborator

@binh-dam-ibigroup binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you clean up the effect, the reference to map actions, and the types?

lib/components/map/connected-geojson-layer.tsx Show resolved Hide resolved
lib/components/map/connected-geojson-layer.tsx Show resolved Hide resolved
lib/components/map/connected-geojson-layer.tsx Outdated Show resolved Hide resolved
lib/components/map/connected-geojson-layer.tsx Show resolved Hide resolved
lib/components/map/connected-geojson-layer.js Outdated
<img
alt={properties.Name}
src={properties.icon}
style={{ height: 40 }}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a className should still be set on the <img> tag so that the configs can style the images if needed.

binh-dam-ibigroup
binh-dam-ibigroup approved these changes Dec 8, 2023
View reviewed changes
Copy link
Collaborator

@binh-dam-ibigroup binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing my comments. Let's give it a go!

@miles-grant-ibigroup miles-grant-ibigroup merged commit 1258a03 into dev Dec 8, 2023
9 checks passed
@miles-grant-ibigroup miles-grant-ibigroup deleted the geojson-layer branch December 8, 2023 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amy-corson-ibigroup amy-corson-ibigroup amy-corson-ibigroup approved these changes

@binh-dam-ibigroup binh-dam-ibigroup binh-dam-ibigroup approved these changes

Assignees

@miles-grant-ibigroup miles-grant-ibigroup

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@miles-grant-ibigroup @binh-dam-ibigroup @amy-corson-ibigroup